Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sarek bcftools normalization #1682

Open
wants to merge 66 commits into
base: dev
Choose a base branch
from

Conversation

Patricie34
Copy link

@Patricie34 Patricie34 commented Oct 9, 2024

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@Patricie34
Copy link
Author

Hi all,

I've modified the normalization step to include all VCFs, not just the germline ones. For this, I used the pull request from JC-Delmas as a base. I am aware that this still requires a lot of work, and I would greatly appreciate any advice or feedback you can provide.

Thank you!

Patricie

Copy link

github-actions bot commented Oct 10, 2024

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 6c39a63

+| ✅ 215 tests passed       |+
#| ❔  11 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.2
  • Run at 2024-12-04 06:57:12

@maxulysse
Copy link
Member

@nf-core-bot fix linting pretty please 🙏

@maxulysse
Copy link
Member

We're missing CHANGELOG + tests + subway map

@maxulysse
Copy link
Member

@nf-core-bot fix linting pretty please 🙏

nextflow Outdated Show resolved Hide resolved
Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! Now only the fun little side quests are missing:

  • Updating the readme to include this in the possible steps
  • Updating the subway map
  • Add a section to the output.md docs

@nf-core-bot
Copy link
Member

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 3.0.2.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation.

@maxulysse
Copy link
Member

@Patricie34 sorry, I forgot to merge in #1760 (which is now done).

Can you sync you branch once more, and move your PR up in the CHANGELOG?

maxulysse
maxulysse previously approved these changes Jan 7, 2025
README.md Outdated Show resolved Hide resolved
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good for me

Comment on lines +61 to +62
ext.prefix = { "${input.baseName}" }
ext.when = { params.concatenate_vcfs || params.normalize_vcfs }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ext.prefix = { "${input.baseName}" }
ext.when = { params.concatenate_vcfs || params.normalize_vcfs }
ext.prefix = { "${input.baseName}" }
ext.when = { params.concatenate_vcfs || params.normalize_vcfs }

@@ -22,7 +22,7 @@ workflow CONCATENATE_GERMLINE_VCFS {
TABIX_EXT_VCF(ADD_INFO_TO_VCF.out.vcf)

// Gather vcfs and vcf-tbis for concatenating germline-vcfs
germline_vcfs_with_tbis = TABIX_EXT_VCF.out.gz_tbi.map{ meta, vcf, tbi -> [ meta.subMap('id'), vcf, tbi ] }.groupTuple()
germline_vcfs_with_tbis = TABIX_EXT_VCF.out.gz_tbi.groupTuple()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this? This was supposed to group all vcfs together. aren't they now not divided by their sample name?

@@ -183,6 +184,7 @@ We thank the following people for their extensive assistance in the development
- [Szilveszter Juhos](https://github.com/szilvajuhos)
- [Tobias Koch](https://github.com/KochTobi)
- [Winni Kretzschmar](https://github.com/winni2k)
- [Patricie Skaláková](https://github.com/Patricie34)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alphabetical order 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants